blocksuite/affine/gfx/pointer/src/snap/snap-overlay.ts (1)

54-84: Tighten numeric guard + avoid recreating the interval constant per call

The snapping logic is good, but I’d recommend (1) guarding on distance with a small epsilon (instead of dx === 0 && dy === 0), (2) fixing the “division by zero” comment, and (3) hoisting the interval to a static readonly (or module constant) for reuse. Also consider whether returning currentPoint by reference (when Shift isn’t pressed) is intended vs always returning a new Point for consistency.

Proposed diff

 export class SnapOverlay extends Overlay {
   static override overlayName: string = 'snap-manager';
 
+  private static readonly ANGLE_SNAP_INTERVAL_RAD = Math.PI / 12; // 15 degrees
+  private static readonly SNAP_EPSILON = 1e-6;
+
   private _skippedElements: Set<GfxModel> = new Set();
@@
   snapDragAngle(startPoint: Point, currentPoint: Point, isShiftPressed: boolean): Point {
     if (!isShiftPressed) {
       return currentPoint;
     }
 
-    const ANGLE_SNAP_INTERVAL_RAD = Math.PI / 12; // 15 degrees in radians
-
     const dx = currentPoint.x - startPoint.x;
     const dy = currentPoint.y - startPoint.y;
 
-    if (dx === 0 && dy === 0) {
-      return currentPoint; // Avoid division by zero when points are identical
-    }
+    const distance = Math.hypot(dx, dy);
+    if (distance < SnapOverlay.SNAP_EPSILON) {
+      return currentPoint; // No effective movement; avoid snapping jitter
+    }
 
     const currentAngle = Math.atan2(dy, dx);
-    const snappedAngle = Math.round(currentAngle / ANGLE_SNAP_INTERVAL_RAD) * ANGLE_SNAP_INTERVAL_RAD;
-    const distance = Math.sqrt(dx * dx + dy * dy);
+    const interval = SnapOverlay.ANGLE_SNAP_INTERVAL_RAD;
+    const snappedAngle = Math.round(currentAngle / interval) * interval;
 
     const snappedX = startPoint.x + distance * Math.cos(snappedAngle);
     const snappedY = startPoint.y + distance * Math.sin(snappedAngle);
 
     return new Point(snappedX, snappedY);
   }

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)

65-70: Make snapOverlay readonly.

snapOverlay is assigned only in the constructor and never reassigned, so it should be readonly. While SnapOverlay inherits a dispose() method from its parent Overlay class, it does not currently attach listeners or allocate resources requiring cleanup. However, for consistency with the extension lifecycle pattern, consider calling this.snapOverlay.dispose() in InteractivityManager.unmounted() if resource management needs become necessary in the future.

Proposed change

-  private snapOverlay: SnapOverlay;
+  private readonly snapOverlay: SnapOverlay;

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

blocksuite/affine/gfx/pointer/src/__tests__/snap-overlay.unit.spec.ts (3)

46-51: Clarify the test and verify distance preservation.

The trigonometric expression 10 * Math.tan(Math.PI / 4 + Math.PI / 100) is complex and could be clearer. Consider extracting it to a named constant or adding a comment explaining it represents a point near 45°.

Additionally, similar to the 0° test, verify that the expected snapped point (10, 10) matches the implementation's distance-preserving behavior. The original distance is sqrt(100 + (10*tan(46.8°))^2) ≈ 14.55, so the snapped point should be approximately (10.29, 10.29) to preserve distance.

---

78-83: Consider improving test clarity.

The test correctly verifies snapping to -15° (345°), but the inline trigonometric calculations make it harder to read. Consider extracting the angle and radius to named constants:

const targetAngle = -Math.PI / 12; // -15 degrees
const offsetAngle = -Math.PI / 100; // small offset for testing snap behavior
const radius = 10;
const inputAngle = targetAngle + offsetAngle;
const currentPoint = new Point(
  radius * Math.cos(inputAngle),
  radius * Math.sin(inputAngle)
);

This would make the test's intent clearer without changing its behavior.

---

22-84: Consider expanding test coverage for other 15° increments.

The test suite covers key angles (0°, 45°, 90°, 180°, -15°), but since the PR implements snapping to 15° increments, consider adding tests for other multiples like 15°, 30°, 60°, 75°, 120°, 135°, etc. This would provide more comprehensive verification that the snapping works correctly across all angle ranges.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)

400-408: Consider simplifying the redundant boolean parameter.

The snapDragAngle method is only called when isShiftPressed is true (Line 404), yet the third parameter is hardcoded to true (Line 405). Consider one of these approaches:

1. Pass the isShiftPressed variable directly: snapDragAngle(internal.dragStartPos, dragLastPos, isShiftPressed)
2. Remove the conditional and always call snapDragAngle, letting it handle the shift key state internally
3. If the API requires the boolean, this is fine but slightly redundant

♻️ Proposed simplification

-const isShiftPressed = this.keyboard.shiftKey$.peek();
-if (isShiftPressed) {
-  const snappedPoint = this.snapOverlay.snapDragAngle(internal.dragStartPos, dragLastPos, true);
+if (this.keyboard.shiftKey$.peek()) {
+  const snappedPoint = this.snapOverlay.snapDragAngle(internal.dragStartPos, dragLastPos, this.keyboard.shiftKey$.peek());
   dx = snappedPoint.x - internal.dragStartPos.x;
   dy = snappedPoint.y - internal.dragStartPos.y;
 }

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)

400-408: Remove redundant shift key check.

The code evaluates this.keyboard.shiftKey$.peek() twice: once in the if condition (line 404) and again when passing it to snapDragAngle (line 405). According to the SnapOverlay implementation, snapDragAngle returns the original currentPoint when isShiftPressed is false, making the outer if check unnecessary.

♻️ Suggested simplification

Option 1 (recommended): Remove the if check and always call snapDragAngle

-      let dx = dragLastPos.x - internal.dragStartPos.x;
-      let dy = dragLastPos.y - internal.dragStartPos.y;
-
-      
-      if (this.keyboard.shiftKey$.peek()) {
-          const snappedPoint = this.snapOverlay.snapDragAngle(internal.dragStartPos, dragLastPos, this.keyboard.shiftKey$.peek());
-        dx = snappedPoint.x - internal.dragStartPos.x;
-        dy = snappedPoint.y - internal.dragStartPos.y;
-      }
+      const snappedPoint = this.snapOverlay.snapDragAngle(
+        internal.dragStartPos,
+        dragLastPos,
+        this.keyboard.shiftKey$.peek()
+      );
+      const dx = snappedPoint.x - internal.dragStartPos.x;
+      const dy = snappedPoint.y - internal.dragStartPos.y;

Option 2: Pass true directly since we know shift is pressed

       let dx = dragLastPos.x - internal.dragStartPos.x;
       let dy = dragLastPos.y - internal.dragStartPos.y;
-
-      
+
       if (this.keyboard.shiftKey$.peek()) {
-          const snappedPoint = this.snapOverlay.snapDragAngle(internal.dragStartPos, dragLastPos, this.keyboard.shiftKey$.peek());
+        const snappedPoint = this.snapOverlay.snapDragAngle(internal.dragStartPos, dragLastPos, true);
         dx = snappedPoint.x - internal.dragStartPos.x;
         dy = snappedPoint.y - internal.dragStartPos.y;
       }

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)

441-452: Snapping consistency is now correct; minor indentation inconsistency.

The snapping logic in onDragEnd now matches onDragMove, addressing the previously flagged issue. However, lines 445–452 appear to use 5-space indentation while the equivalent block in onDragMove (lines 399–405) uses 6 spaces. Consider aligning for consistency.

♻️ Suggested fix

-      const snappedPoint = this.snapOverlay.snapDragAngle(
-        internal.dragStartPos,
-        dragLastPos,
-        this.keyboard.shiftKey$.peek()
-     );
-     const endContext: ExtensionDragEndContext = {
+      const snappedPoint = this.snapOverlay.snapDragAngle(
+        internal.dragStartPos,
+        dragLastPos,
+        this.keyboard.shiftKey$.peek()
+      );
+      const endContext: ExtensionDragEndContext = {
         ...internal,
         event,
         dragLastPos,
-        dx: snappedPoint.x - internal.dragStartPos.x,
-        dy: snappedPoint.y - internal.dragStartPos.y,
-     };
+        dx: snappedPoint.x - internal.dragStartPos.x,
+        dy: snappedPoint.y - internal.dragStartPos.y,
+      };

blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)

404-410: Guard against undefined snapOverlay in drag move handler.

snapOverlay is declared as optional (line 65), but line 404 accesses it directly without optional chaining. The drag end handler (line 446) correctly uses ?. but the drag move handler doesn't.

🔧 Suggested fix: Add optional chaining with fallback

-      const snappedPoint = this.snapOverlay.snapDragAngle(
+      const snappedPoint = this.snapOverlay?.snapDragAngle(
         internal.dragStartPos,
         dragLastPos,
         this.keyboard.shiftKey$.peek()
-      );
+      ) ?? dragLastPos;
       const dx = snappedPoint.x - internal.dragStartPos.x;
       const dy = snappedPoint.y - internal.dragStartPos.y;

blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)

446-460: Redundant null-coalescing on finalPoint.

Line 450 already guarantees snappedPoint is non-nullish via ?? dragLastPos. Line 452's snappedPoint ?? dragLastPos is therefore redundant and adds confusion.

♻️ Suggested simplification

       const snappedPoint = this.snapOverlay?.snapDragAngle(
         internal.dragStartPos,
         dragLastPos,
         this.keyboard.shiftKey$.peek()
        ) ?? dragLastPos;
 
-      const finalPoint = snappedPoint ?? dragLastPos;
-      
       const endContext: ExtensionDragEndContext = {
         ...internal,
         event,
-        dragLastPos: finalPoint,
-        dx: finalPoint.x - internal.dragStartPos.x,
-        dy: finalPoint.y - internal.dragStartPos.y,
+        dragLastPos: snappedPoint,
+        dx: snappedPoint.x - internal.dragStartPos.x,
+        dy: snappedPoint.y - internal.dragStartPos.y,
        };

blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)

401-407: Guard against undefined snapOverlay in drag move handler.

snapOverlay is declared as optional (?), but line 401 accesses it directly without optional chaining. The drag end handler (line 443) correctly uses this.snapOverlay?.snapDragAngle(...) ?? dragLastPos, but the drag move handler does not. This inconsistency could cause a runtime error if onDragMove is invoked before mounted().

🔧 Suggested fix: Add optional chaining with fallback

-      const snappedPoint = this.snapOverlay.snapDragAngle(
+      const snappedPoint = this.snapOverlay?.snapDragAngle(
         internal.dragStartPos,
         dragLastPos,
         this.keyboard.shiftKey$.peek()
-      );
+      ) ?? dragLastPos;
       const dx = snappedPoint.x - internal.dragStartPos.x;
       const dy = snappedPoint.y - internal.dragStartPos.y;

blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)

443-457: Remove redundant finalPoint variable.

snappedPoint at line 443-447 already has a fallback: this.snapOverlay?.snapDragAngle(...) ?? dragLastPos. Therefore, snappedPoint is guaranteed to be defined, making const finalPoint = snappedPoint ?? dragLastPos at line 449 redundant—the ?? dragLastPos branch will never execute.

Also note the inconsistent indentation at line 457 (extra leading space on the closing brace).

♻️ Suggested cleanup

       const snappedPoint = this.snapOverlay?.snapDragAngle(
         internal.dragStartPos,
         dragLastPos,
         this.keyboard.shiftKey$.peek()
        ) ?? dragLastPos;
-
-      const finalPoint = snappedPoint ?? dragLastPos;
-      
       const endContext: ExtensionDragEndContext = {
         ...internal,
         event,
-        dragLastPos: finalPoint,
-        dx: finalPoint.x - internal.dragStartPos.x,
-        dy: finalPoint.y - internal.dragStartPos.y,
-       };
+        dragLastPos: snappedPoint,
+        dx: snappedPoint.x - internal.dragStartPos.x,
+        dy: snappedPoint.y - internal.dragStartPos.y,
+      };

blocksuite/framework/std/src/gfx/interactivity/manager.ts (2)

71-76: Minor: Extra blank lines after mounted() method.

Lines 75-76 have consecutive blank lines which is inconsistent with the rest of the file's style.

♻️ Suggested fix

   mounted() {
     this.snapOverlay = new SnapOverlay(this.gfx);
     this.canvasEventHandler = new GfxViewEventManager(this.gfx);
   }
-  
-  
+
   activeInteraction$ = signal<null | {

---

442-456: Redundant nullish coalescing and indentation inconsistency.

1. Line 448 (const finalPoint = snappedPoint ?? dragLastPos;) is redundant - snappedPoint is already guaranteed to be non-nullish from line 446's fallback ?? dragLastPos.

2. Indentation is inconsistent around lines 446 and 456.

♻️ Suggested cleanup

       const snappedPoint = this.snapOverlay?.snapDragAngle(
         internal.dragStartPos,
         dragLastPos,
         this.keyboard.shiftKey$.peek()
-       ) ?? dragLastPos;
-
-      const finalPoint = snappedPoint ?? dragLastPos;
-      
+      ) ?? dragLastPos;
+
       const endContext: ExtensionDragEndContext = {
         ...internal,
         event,
-        dragLastPos: finalPoint,
-        dx: finalPoint.x - internal.dragStartPos.x,
-        dy: finalPoint.y - internal.dragStartPos.y,
-       };
+        dragLastPos: snappedPoint,
+        dx: snappedPoint.x - internal.dragStartPos.x,
+        dy: snappedPoint.y - internal.dragStartPos.y,
+      };

blocksuite/framework/std/src/gfx/interactivity/manager.ts (3)

442-456: Redundant null coalescing on finalPoint.

Line 448 (const finalPoint = snappedPoint ?? dragLastPos) is redundant because snappedPoint is already guaranteed to be non-nullish from the null coalescing on line 446. The ?? dragLastPos fallback already ensures snappedPoint is either the snap result or dragLastPos.

♻️ Suggested simplification

       const snappedPoint = this.snapOverlay?.snapDragAngle(
         internal.dragStartPos,
         dragLastPos,
         this.keyboard.shiftKey$.peek()
        ) ?? dragLastPos;

-      const finalPoint = snappedPoint ?? dragLastPos;
-      
       const endContext: ExtensionDragEndContext = {
         ...internal,
         event,
-        dragLastPos: finalPoint,
-        dx: finalPoint.x - internal.dragStartPos.x,
-        dy: finalPoint.y - internal.dragStartPos.y,
+        dragLastPos: snappedPoint,
+        dx: snappedPoint.x - internal.dragStartPos.x,
+        dy: snappedPoint.y - internal.dragStartPos.y,
        };

---

983-1001: Static analysis: forEach callback should not return a value.

Per Biome lint, the forEach callback at line 985 implicitly returns the result of ext.onResizeStart?.(...). While this doesn't cause runtime issues (forEach ignores return values), it's cleaner to use an explicit block body.

♻️ Suggested fix

-      extensionHandlers.forEach(ext =>
-        ext.onResizeStart?.({ elements, handle, handlePos, handleSign })
-      );
+      extensionHandlers.forEach(ext => {
+        ext.onResizeStart?.({ elements, handle, handlePos, handleSign });
+      });

---

1028-1033: Static analysis: Same forEach return value issue.

Same pattern as line 985 - the callback implicitly returns a value that forEach ignores.

♻️ Suggested fix

-      extensionHandlers.forEach(ext =>
-        ext.onResizeEnd?.({ elements, handle, handlePos, handleSign })
-      );
+      extensionHandlers.forEach(ext => {
+        ext.onResizeEnd?.({ elements, handle, handlePos, handleSign });
+      });

blocksuite/framework/std/src/gfx/interactivity/manager.ts (2)

442-456: Redundant null coalescing on finalPoint.

snappedPoint is already assigned with a fallback to dragLastPos on lines 442-446, so it will never be nullish. The ?? dragLastPos on line 448 is dead code.

♻️ Suggested simplification

       const snappedPoint = this.snapOverlay?.snapDragAngle(
         internal.dragStartPos,
         dragLastPos,
         this.keyboard.shiftKey$.peek()
        ) ?? dragLastPos;

-      const finalPoint = snappedPoint ?? dragLastPos;
-      
       const endContext: ExtensionDragEndContext = {
         ...internal,
         event,
-        dragLastPos: finalPoint,
-        dx: finalPoint.x - internal.dragStartPos.x,
-        dy: finalPoint.y - internal.dragStartPos.y,
+        dragLastPos: snappedPoint,
+        dx: snappedPoint.x - internal.dragStartPos.x,
+        dy: snappedPoint.y - internal.dragStartPos.y,
        };

---

983-988: forEach callback implicitly returns a value (flagged by static analysis).

The arrow function ext => ext.onResizeStart?.(...) implicitly returns the result of the optional call. While harmless, this triggers Biome's useIterableCallbackReturn lint rule. Wrap in braces to suppress.

♻️ Suggested fix

       extensionHandlers.forEach(ext => {
-        ext.onResizeStart?.({ elements, handle, handlePos, handleSign })
+        ext.onResizeStart?.({ elements, handle, handlePos, handleSign });
       });

The same pattern applies to line 1030 (onResizeEnd).

blocksuite/framework/std/src/gfx/interactivity/manager.ts (2)

442-456: Redundant null coalescing and inconsistent indentation.

Line 448 (const finalPoint = snappedPoint ?? dragLastPos) is redundant since snappedPoint on line 446 already has ?? dragLastPos as a fallback, making it impossible for snappedPoint to be nullish.

Additionally, the indentation at lines 446 and 456 is inconsistent with the surrounding code.

♻️ Suggested cleanup

       const snappedPoint = this.snapOverlay?.snapDragAngle(
         internal.dragStartPos,
         dragLastPos,
         this.keyboard.shiftKey$.peek()
-       ) ?? dragLastPos;
-
-      const finalPoint = snappedPoint ?? dragLastPos;
-      
+      ) ?? dragLastPos;
+
       const endContext: ExtensionDragEndContext = {
         ...internal,
         event,
-        dragLastPos: finalPoint,
-        dx: finalPoint.x - internal.dragStartPos.x,
-        dy: finalPoint.y - internal.dragStartPos.y,
-       };
+        dragLastPos: snappedPoint,
+        dx: snappedPoint.x - internal.dragStartPos.x,
+        dy: snappedPoint.y - internal.dragStartPos.y,
+      };

---

1030-1032: Inconsistent indentation in forEach callback.

Lines 1031-1032 have inconsistent indentation compared to the surrounding code. The closing brace should align with the opening of the forEach.

♻️ Suggested fix

       extensionHandlers.forEach(ext => {
-       ext.onResizeEnd?.({ elements, handle, handlePos, handleSign });
-     });
+        ext.onResizeEnd?.({ elements, handle, handlePos, handleSign });
+      });

blocksuite/framework/std/src/gfx/interactivity/manager.ts (1)

442-456: Remove redundant null coalescing operator.

snappedPoint is already assigned with a fallback (?? dragLastPos) on line 446, so it can never be null or undefined. The subsequent const finalPoint = snappedPoint ?? dragLastPos; on line 448 is redundant dead code.

♻️ Proposed fix

       const snappedPoint = this.snapOverlay?.snapDragAngle(
         internal.dragStartPos,
         dragLastPos,
         this.keyboard.shiftKey$.peek()
-       ) ?? dragLastPos;
-
-      const finalPoint = snappedPoint ?? dragLastPos;
-      
+      ) ?? dragLastPos;
+
       const endContext: ExtensionDragEndContext = {
         ...internal,
         event,
-        dragLastPos: finalPoint,
-        dx: finalPoint.x - internal.dragStartPos.x,
-        dy: finalPoint.y - internal.dragStartPos.y,
-       };
+        dragLastPos: snappedPoint,
+        dx: snappedPoint.x - internal.dragStartPos.x,
+        dy: snappedPoint.y - internal.dragStartPos.y,
+      };

blocksuite/framework/std/src/gfx/interactivity/manager.ts (2)

442-456: Remove redundant null coalescing.

snappedPoint already has a fallback to dragLastPos on line 446, so it can never be null or undefined. The finalPoint assignment on line 448 is redundant and adds unnecessary complexity.

♻️ Suggested simplification

       const snappedPoint = this.snapOverlay?.snapDragAngle(
         internal.dragStartPos,
         dragLastPos,
         this.keyboard.shiftKey$.peek()
        ) ?? dragLastPos;

-      const finalPoint = snappedPoint ?? dragLastPos;
-      
       const endContext: ExtensionDragEndContext = {
         ...internal,
         event,
-        dragLastPos: finalPoint,
-        dx: finalPoint.x - internal.dragStartPos.x,
-        dy: finalPoint.y - internal.dragStartPos.y,
+        dragLastPos: snappedPoint,
+        dx: snappedPoint.x - internal.dragStartPos.x,
+        dy: snappedPoint.y - internal.dragStartPos.y,
        };

---

1053-1057: Inconsistent indentation.

Lines 1056-1057 have extra leading whitespace compared to the surrounding code structure. This is a minor formatting issue.

🎨 Suggested fix

       extensionHandlers.forEach(ext => {
-       ext.onResizeEnd?.({ elements, handle, handlePos, handleSign });
-     });
+        ext.onResizeEnd?.({ elements, handle, handlePos, handleSign });
+      });

blocksuite/framework/std/src/gfx/interactivity/manager.ts (3)

67-69: Unnecessary constructor.

This constructor only calls super(...args) with no additional logic. TypeScript will generate this automatically.

♻️ Suggested removal

-  constructor(...args: ConstructorParameters<typeof GfxExtension>) {
-    super(...args);
-  }

---

517-612: Inconsistent indentation in handleElementRotate.

The method body is indented at 1-space/2-space from the class body instead of the standard indentation level used by other methods (e.g., handleElementMove, handleElementResize). This appears to be an accidental shift, making the code harder to read.

---

1059-1061: Inconsistent indentation in onResizeEnd callback.

Lines 1060-1061 use mismatched indentation (extra leading space on 1060, reduced on 1061) compared to surrounding code.

🧹 Fix indentation

       extensionHandlers.forEach(ext => {
-       ext.onResizeEnd?.({ elements, handle, handlePos, handleSign });
-     });
+        ext.onResizeEnd?.({ elements, handle, handlePos, handleSign });
+      });